Skip to content

[ETCM-912] Magneto gas changes for BALANCE and *CALL codes #1084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 9, 2021

Conversation

lukasz-golebiewski
Copy link
Contributor

No description provided.

@lukasz-golebiewski lukasz-golebiewski force-pushed the feature/etcm-921/more-codes branch from 9bcbb6e to aa23ecf Compare July 30, 2021 13:21
@drospa drospa changed the title [ETCM-921] Magneto gas changes for BALANCE and *CALL codes [ETCM-912] Magneto gas changes for BALANCE and *CALL codes Aug 2, 2021
@@ -191,7 +191,8 @@ class CallOpFixture(val config: EvmConfig, val startState: MockWorldState) {
inOffset: UInt256 = UInt256.Zero,
inSize: UInt256 = inputData.size,
outOffset: UInt256 = inputData.size,
outSize: UInt256 = inputData.size / 2
outSize: UInt256 = inputData.size / 2,
toAccessed: Boolean = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I had to look into the code to understand what this field was about.
I would have prefered something more descriptive like addAddressToAccessList or keepTrackOfAccessAddresses, but I tend to belong to the long-name-team.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about toAlreadyAccessed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems clearer for me.


import Fixtures.blockchainConfig

class MagnetoCallOpFixture(config: EvmConfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the tests seem to have be taken directly from CallOpcodesSpec.
It could be nice to keep unmodified tests in one place only, and have specific gas ones separated.

It seems to be possible to have something like that:

  • CallOpCodesCommonBehaviour: with the common tests
  • CallOpCodesSpec: with the pre eip2929 gas tests
  • CallOpCodesSpecPostEip2929: with post eip 2929 gas tests

This kind of separation seems to be possible: https://www.scalatest.org/user_guide/sharing_tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reduced as much duplication as possible, moved common assertions to a trait

postWarmGasFn: FeeSchedule => BigInt
): BigInt = {
val currentBlockNumber = state.env.blockHeader.number
val etcFork = state.config.blockchainConfig.etcForkForBlockNumber(currentBlockNumber)
Copy link
Contributor

@dzajkowski dzajkowski Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mind adding a note that ETH needs to be also handled?

Copy link
Contributor

@dzajkowski dzajkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lukasz-golebiewski lukasz-golebiewski force-pushed the feature/etcm-921/more-codes branch from aa23ecf to 3740dbd Compare August 9, 2021 07:18
@lukasz-golebiewski lukasz-golebiewski force-pushed the feature/etcm-921/more-codes branch from 000dc2d to e487da7 Compare August 9, 2021 13:22
@lukasz-golebiewski lukasz-golebiewski merged commit 241d1f3 into develop Aug 9, 2021
@lukasz-golebiewski lukasz-golebiewski deleted the feature/etcm-921/more-codes branch August 9, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants